-
Notifications
You must be signed in to change notification settings - Fork 8k
UVC: move application decision to the application #93192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
UVC: move application decision to the application #93192
Conversation
This is not ready for review, will be properly split in individual commits, and "un-drafted" when ready... |
Force-push:
Windows, Linux, MacOSX, Android are now expected to all work. Tested with nRF52840-DK:
Tested with Arduino Nicla Vision:
|
f1cb80e
to
f5328dd
Compare
Force-push:
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs rebase.
2630948
to
9180180
Compare
Force-push:
|
9180180
to
ee542de
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks a lot !!
@jfischer-no, @tmon-nordic, could you have a look at this PR which is a dependency for some others ? Thanks. |
include/zephyr/usb/class/usbd_uvc.h
Outdated
* At runtime, it will forward all USB controls from the host to this device. | ||
* It will query its supported video controls and frame intervals and use this information to | ||
* generate the USB descriptors presented to the host. At runtime, it will forward all USB controls | ||
* from the host to this device. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At runtime, it will forward all USB controls from the host to this device.
How would this function "forward all USB controls" and why? I doubt this documentation is correct. Perhaps it needs different wording, also not clear how the changes are relevant to the commit message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant was that the class would turn USB control commands for the UVC class into "video controls" at the Zephyr Video API level.
Good opportunity to improve the inaccurate documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized that I did integrate @avolmat-st suggestions into existing commits, I just split them out like I should have.
ee542de
to
915e75b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some nits, otherwise LGTM.
samples/subsys/usb/uvc/src/main.c
Outdated
/* Format capabilities of video_dev, used everywhere through the sample */ | ||
static struct video_caps video_caps = {.type = VIDEO_BUF_TYPE_OUTPUT}; | ||
|
||
static void app_add_format(uint32_t pixfmt, uint16_t width, uint16_t height) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
width and height parameters should be uint32_t as in video_format structure. The calls to this function (line 57 and 60) also passes uint32_t type from video_format_caps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I see uint16_t
is OK as well, uint32_t
is a bit overkill. Maybe we should change the video_format_caps
and video_format
's width / height fields to uint16_t
in the future ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I must have been too used to UVC sizes... Done.
samples/subsys/usb/uvc/src/main.c
Outdated
}; | ||
int ret; | ||
|
||
/* Set the format to get the pitch */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be /* Set the format to get the size */
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the comment was not updated since fmt.size introduction. Done.
samples/subsys/usb/uvc/src/main.c
Outdated
/* Set the format to get the pitch */ | ||
ret = video_set_format(video_dev, &fmt); | ||
if (ret != 0) { | ||
LOG_ERR("Could not set the format of %s", video_dev->name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should also log the fmt (size, width, height) to give more info about which format has failed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. Done.
include/zephyr/usb/class/usbd_uvc.h
Outdated
void uvc_set_video_dev(const struct device *uvc_dev, const struct device *video_dev); | ||
|
||
/** | ||
* @brief Set the video format capabilities that a UVC instance will present to the host. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function description is confused to me. The function rather add a format, not set the video format caps, right ?
@brief Add a video format ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this got refactored and the comment did not get updated properly. Done.
include/zephyr/usb/class/usbd_uvc.h
Outdated
* @param uvc_dev The UVC device to configure | ||
* @param fmt The video format to add to this UVC instance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@param Pointer to ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done here too.
include/zephyr/usb/class/usbd_uvc.h
Outdated
* | ||
* @param uvc_dev The UVC device | ||
* @param video_dev The video device that this UVC instance controls | ||
* @param video_dev The video device that this UVC instance send controls requests to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@param Pointer to ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated here and other places.
samples/subsys/usb/uvc/src/main.c
Outdated
|
||
ret = video_set_format(video_dev, &fmt); | ||
if (ret != 0) { | ||
LOG_WRN("Could not set the format of %s", video_dev->name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should also log the fmt (size, width, height) to give more info about which format has failed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done here too.
samples/subsys/usb/uvc/src/main.c
Outdated
static struct video_caps video_caps = {.type = VIDEO_BUF_TYPE_OUTPUT}; | ||
|
||
static void app_add_format(uint32_t pixfmt, uint16_t width, uint16_t height) | ||
static bool app_is_standard_format(uint32_t pixfmt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: app_is_supported_format()
? as the definition of which formats are "standard" seems not clear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the UVC standard
USB_Video_Payload_Uncompressed_1.5.pdf
But the list is already outdated and incomplete!
I added NV12.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed GREY
, added NV12
, and renamed the function.
Glad to pick a different strategy!
samples/subsys/usb/uvc/src/main.c
Outdated
} | ||
|
||
/* Check whether the video device supports one of the wisespread image sensor formats */ | ||
static bool app_has_standard_formats(void) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit : app_has_supported_formats()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied there too.
For supporting situations where UVC formats are dynamically generated at runtime, do not return an error in case descriptors were not added, but instead verbosely log the error and allow enumeration to happen. Signed-off-by: Josuah Demangeon <[email protected]>
Make use of the recently merged fmt->size to know the maximum size of the frame to be allocated, which works for both compressed and uncompressed video. Signed-off-by: Josuah Demangeon <[email protected]>
915e75b
to
104a3f6
Compare
The UVC class was deciding itself which formats were sent to the host. Remove this logic out of the UVC class and introduce uvc_add_format() to give the application the freedom of which format to list. Signed-off-by: Josuah Demangeon <[email protected]>
The UVC class now lets the application select the format list sent to the host. Leverage this in the sample to filter out any format that is not expected to work (buffer too big, rarely supported formats). Signed-off-by: Josuah Demangeon <[email protected]>
Add USB UVC device's new uvc_add_format() function to the release note, and document the semantic changes of UVC APIs in the migration guide. Signed-off-by: Josuah Demangeon <[email protected]>
104a3f6
to
95cc035
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Force-push:
- Rebased on
main
- Applied @ngphibang suggestions
samples/subsys/usb/uvc/src/main.c
Outdated
/* Format capabilities of video_dev, used everywhere through the sample */ | ||
static struct video_caps video_caps = {.type = VIDEO_BUF_TYPE_OUTPUT}; | ||
|
||
static void app_add_format(uint32_t pixfmt, uint16_t width, uint16_t height) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I must have been too used to UVC sizes... Done.
samples/subsys/usb/uvc/src/main.c
Outdated
}; | ||
int ret; | ||
|
||
/* Set the format to get the pitch */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the comment was not updated since fmt.size introduction. Done.
samples/subsys/usb/uvc/src/main.c
Outdated
/* Set the format to get the pitch */ | ||
ret = video_set_format(video_dev, &fmt); | ||
if (ret != 0) { | ||
LOG_ERR("Could not set the format of %s", video_dev->name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. Done.
samples/subsys/usb/uvc/src/main.c
Outdated
|
||
ret = video_set_format(video_dev, &fmt); | ||
if (ret != 0) { | ||
LOG_WRN("Could not set the format of %s", video_dev->name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done here too.
include/zephyr/usb/class/usbd_uvc.h
Outdated
void uvc_set_video_dev(const struct device *uvc_dev, const struct device *video_dev); | ||
|
||
/** | ||
* @brief Set the video format capabilities that a UVC instance will present to the host. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this got refactored and the comment did not get updated properly. Done.
include/zephyr/usb/class/usbd_uvc.h
Outdated
* | ||
* @param uvc_dev The UVC device | ||
* @param video_dev The video device that this UVC instance controls | ||
* @param video_dev The video device that this UVC instance send controls requests to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated here and other places.
include/zephyr/usb/class/usbd_uvc.h
Outdated
* @param uvc_dev The UVC device to configure | ||
* @param fmt The video format to add to this UVC instance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done here too.
samples/subsys/usb/uvc/src/main.c
Outdated
static struct video_caps video_caps = {.type = VIDEO_BUF_TYPE_OUTPUT}; | ||
|
||
static void app_add_format(uint32_t pixfmt, uint16_t width, uint16_t height) | ||
static bool app_is_standard_format(uint32_t pixfmt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the UVC standard
USB_Video_Payload_Uncompressed_1.5.pdf
But the list is already outdated and incomplete!
I added NV12.
samples/subsys/usb/uvc/src/main.c
Outdated
static struct video_caps video_caps = {.type = VIDEO_BUF_TYPE_OUTPUT}; | ||
|
||
static void app_add_format(uint32_t pixfmt, uint16_t width, uint16_t height) | ||
static bool app_is_standard_format(uint32_t pixfmt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed GREY
, added NV12
, and renamed the function.
Glad to pick a different strategy!
samples/subsys/usb/uvc/src/main.c
Outdated
} | ||
|
||
/* Check whether the video device supports one of the wisespread image sensor formats */ | ||
static bool app_has_standard_formats(void) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied there too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
|
Dependency:
The USB class was doing arbitrary "best guess" choices about what pixel formats or resolutions to include (all of them, and min/max only when format are ranges).
uvc_add_format()
to allow the application to push the exact format it wantsWhat is added in the sample is a matter of application choices, and trapping these choices in the UVC class prevented the samples to work without manual configuration on diverse platform.
MacOS support still not confirmed.
Linux, Windows, Android tested functional with the hardware I have.